-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add WHEN STALE syntax for CREATE MATERIALIZED VIEW
#27356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| TableHandle tableHandle = metadata.getTableHandle(session, storageTableName) | ||
| .orElseThrow(() -> semanticException(INVALID_VIEW, table, "Storage table '%s' does not exist", storageTableName)); | ||
| return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.of(tableHandle)); | ||
| return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.of(tableHandle), useLogicalViewSemantics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned about this line: the new clause affects the path for fresh MVs (WHEN STALE FAIL skips analysis, while WHEN STALE INLINE runs the analysis), even though the syntax suggests it should only define behavior for stale MVs.
Is analysis of the underlying query even necessary for fresh MVs, regardless of the WHEN STALE clause? I understand that it can detect schema changes in the base tables or changes to the MV definer’s access permissions - but is that the intended behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that it can detect schema changes in the base tables or changes to the MV definer’s access permissions - but is that the intended behavior?
i think that was intended behavior...
Is analysis of the underlying query even necessary for fresh MVs, regardless of the
WHEN STALEclause?
... however i think it should be skipped. this would be significant benefit when MVs are used in low latency scenarios.
in fact, for an MV with 24h grace period that was refreshed with last day, we should skip this line during a SELECT query, because it doesn't matter
trino/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Line 3906 in 0ce5929
| tableChangeInfoTasks.add(() -> getTableChangeInfo(session, tableToSnapShot)); |
let's file an issue for this and follow-up separately
|
What is our plan on changing this behaviour for existing or new MVs ? |
core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java
Show resolved
Hide resolved
| } | ||
| // This is a stale materialized view and should be expanded like a logical view | ||
| return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.empty()); | ||
| return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.empty(), useLogicalViewSemantics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing a useLogicalViewSemantics can we create a scope for the storage table ?
|
Apart from the benefit on skipping the analysis what are the other benefits would I get if a run a query as |
|
For context, #15326 |
|
Also, #23387 (reply in thread) and #23747 |
This change is meant to be backward-compatible. The new clause is optional. If it is not specified, or if
Skipping the analysis is not the goal in itself. The goal is to achieve better availability when the source tables are not available and to get closer to the semantics mentioned in the issues that Martin and I linked in this PR - specifically, that within the grace period, the MV should be treated as if it were a regular storage table. And today, as far as I know, the only blocker for this is the analysis performed every time the MV is queried.
This is more or less what I wanted to express in this comment: #27356 (comment). I’m happy to discuss what the syntax should look like to achieve the goals described above. Personally, |
|
@martint just a gentle reminder about the PR when you have a moment |
|
@piotrrzysko can you please rebase? there is a conflict. |
core/trino-main/src/main/java/io/trino/sql/rewrite/ShowQueriesRewrite.java
Show resolved
Hide resolved
findepi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add WHEN STALE FAIL/INLINE syntax
lgtm
findepi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add when stale behavior in ConnectorMaterializedViewDefinition
core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java
Show resolved
Hide resolved
da71e4a to
8d70622
Compare
WHEN STALE option to CREATE MATERIALIZED VIEWWHEN STALE option to CREATE MATERIALIZED VIEW and implement in Iceberg
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Outdated
Show resolved
Hide resolved
8d70622 to
976cbc3
Compare
WHEN STALE option to CREATE MATERIALIZED VIEW and implement in IcebergWHEN STALE option to CREATE MATERIALIZED VIEW
WHEN STALE option to CREATE MATERIALIZED VIEWWHEN STALE syntax for CREATE MATERIALIZED VIEW
findepi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rebase to resolve a conflict
| if (node.getWhenStaleBehavior().isPresent()) { | ||
| throw new TrinoException(NOT_SUPPORTED, "WHEN STALE is not supported yet"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether Add when stale behavior in ConnectorMaterializedViewDefinition commit should be removing those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to test whether MATERIALIZED_VIEW_WHEN_STALE_BEHAVIOR is respected, which happens during execution:
trino/core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java
Lines 163 to 165 in d9e7986
| if (!plannerContext.getMetadata().getConnectorCapabilities(session, catalogHandle).contains(MATERIALIZED_VIEW_WHEN_STALE_BEHAVIOR)) { | |
| throw semanticException(NOT_SUPPORTED, statement, "Catalog '%s' does not support WHEN STALE", catalogName); | |
| } |
One line below, I throw NOT_SUPPORTED even if a connector claims to support WHEN STALE:
trino/core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java
Line 166 in d9e7986
| throw semanticException(NOT_SUPPORTED, statement, "WHEN STALE is not supported yet"); |
This is a preparatory change that allows connectors to store information about the WHEN STALE behavior. The option cannot yet be used in practice, as execution will still fail with a NOT_SUPPORTED error.
7eeb933 to
d9e7986
Compare
Description
This PR introduces a new optional clause for
CREATE MATERIALIZED VIEWthat allows users to control the behavior of materialized views when they are stale.I will updated the documentation once the proposed syntax and behavior are approved.
New syntax
CREATE MATERIALIZED VIEW mv_name WHEN STALE { FAIL | INLINE } AS SELECT ...Behavior
Motivation
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: